Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: Initialize authentication adapters only once at server start #8464

Open
wants to merge 21 commits into
base: alpha
Choose a base branch
from
Open

perf: Initialize authentication adapters only once at server start #8464

wants to merge 21 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Mar 7, 2023 •
edited
Loading

Pull Request

Issue

Currently, AuthAdapters can be constructed each time a auth call is called. This means validateOptions is called too much.

Closes: #8463

Approach

Loads adapters on server init

Tasks

  • Add tests

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: initialize adapters at server start feat: Initialize adapters at server start Mar 7, 2023
@parse-github-assistant
Copy link

Thanks for opening this pull request!

@dblythy dblythy mentioned this pull request Mar 7, 2023
Draft
1 task
@codecov
Copy link

codecov bot commented Mar 7, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 74.07407% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 94.40%. Comparing base (b07ec15) to head (e8806a3).
Report is 3 commits behind head on alpha.

❗ Current head e8806a3 differs from pull request most recent head a02a0ff. Consider uploading reports for the commit a02a0ff to get more accurate results

Files Patch % Lines
src/Adapters/Auth/AuthAdapter.js 0.00% 5 Missing ⚠️
src/Adapters/Auth/index.js 89.47% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##            alpha    #8464      +/-   ##
==========================================
+ Coverage   94.15%   94.40%   +0.24%     
==========================================
  Files         186      184       -2     
  Lines       14688    14625      -63     
==========================================
- Hits        13829    13806      -23     
+ Misses        859      819      -40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza
Copy link
Member

mtrezza commented Mar 7, 2023 •
edited
Loading

If this a performance improvement or just an code style refactor without significant implications?

@dblythy
Copy link
Member Author

dblythy commented Mar 7, 2023 •
edited
Loading

It should be a minor performance increase, but mostly intended as a functionality change as at the moment errors in auth config are only called once the auth is used.

Most of the old auth adapters are js objects so reconstruction probably has minimal impact. As we move towards making them AuthAdapter classes, preventing reconstruction might be preferable

@mtrezza
Copy link
Member

mtrezza commented Mar 7, 2023

Okay; then let's use perf commit type.

@mtrezza mtrezza changed the title feat: Initialize adapters at server start perf: Initialize authentication adapters only once at server start Mar 7, 2023
Moumouls
Moumouls suggested changes Mar 12, 2023
View reviewed changes
Copy link
Member

@Moumouls Moumouls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr seems complicate to me to just achieve a cache on getValidatorForProvider.

Here you can Object.keys over authOptions of module.exports = function (authOptions = {}, enableAnonymousUsers = true) { and using a factory pre compute each getValidatorForProvider

i also discovered here a source of truth issue about the runAftetFind request object, it will be super hard to maintain the request object if the interface is not linked to a single source of truth. You should refactor this part to use the standard getRequestObject

src/Adapters/Auth/index.js Outdated Show resolved Hide resolved
@dblythy
Copy link
Member Author

dblythy commented May 16, 2023

Cache is required here otherwise each adapter is constructed each time an auth method is called. An auth adapter should be constructed once and then used across the rest of the server instance

@mtrezza mtrezza requested a review from Moumouls May 17, 2023 23:23
mtrezza
mtrezza reviewed May 17, 2023
View reviewed changes
Copy link
Member

@mtrezza mtrezza left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @Moumouls want to give this a quick review before we merge?

The definitions check fails, not sure if that is because I've merged alpha into this branch, or it may be flaky.

Moumouls
Moumouls suggested changes May 25, 2023
View reviewed changes
src/Adapters/Auth/index.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Jun 9, 2023

It seems that the tests are passing; is this ready for merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Moumouls Moumouls Moumouls requested changes

@mtrezza mtrezza mtrezza left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auth adapters are constructed each time RestWrite is called
3 participants
@dblythy @mtrezza @Moumouls